Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Head and wings corrections for k-dTDA #58

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open

Conversation

mkakcl
Copy link
Contributor

@mkakcl mkakcl commented Apr 4, 2024

Implementation of the head and wings corrections for k-dTDA.

Due to a large number of PRs between when this was started and now this PR, have moved all commits relating to the HW corrections from a previous branch to a new branch from master.

@mkakcl mkakcl requested a review from obackhouse April 8, 2024 14:15
Copy link
Contributor

@obackhouse obackhouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments, haven't reviewed in full yet

momentGW/pbc/gw.py Outdated Show resolved Hide resolved
momentGW/pbc/gw.py Outdated Show resolved Hide resolved
momentGW/pbc/gw.py Outdated Show resolved Hide resolved
momentGW/pbc/gw.py Outdated Show resolved Hide resolved
momentGW/pbc/gw.py Outdated Show resolved Hide resolved
momentGW/pbc/tda.py Outdated Show resolved Hide resolved
Copy link
Contributor

@obackhouse obackhouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs the moments functions to be reworked a bit to avoid interrupting original program flow and inheritance, and a few other minor changes

momentGW/pbc/tda.py Outdated Show resolved Hide resolved
momentGW/pbc/tda.py Outdated Show resolved Hide resolved
momentGW/pbc/tda.py Outdated Show resolved Hide resolved
momentGW/pbc/tda.py Outdated Show resolved Hide resolved
momentGW/pbc/tda.py Outdated Show resolved Hide resolved
momentGW/pbc/tda.py Outdated Show resolved Hide resolved
tests/test_kgw.py Show resolved Hide resolved
"""
kpts = self.kpts
head = np.zeros((self.nkpts, self.nmom_max + 1), dtype=object)
HW_const = np.sqrt(4.0 * np.pi) / np.linalg.norm(self.q_abs[0])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no caps in the variable name

momentGW/pbc/tda.py Outdated Show resolved Hide resolved
@mkakcl mkakcl requested a review from obackhouse November 6, 2024 15:38
Copy link
Contributor

@obackhouse obackhouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry that this took so long, the code is good and clean and it didn't really need much more reviewing. Is MPI working? and is test coverage good for your additions? Otherwise, LGTM, go ahead and merge after addressing my minor comments

Comment on lines +128 to +136
if len(list(self.fsc)) > 3:
raise ValueError(
"Finite size corrections require as an input a combination of H, W and B "
"for the different finite size corrections (H - Head, W - Wing, B - Body)")
for i, letter in enumerate(list(self.fsc)):
if letter not in ["H", "W", "B"]:
raise ValueError(
"Finite size corrections require as an input a combination of H, W and B "
"for the different finite size corrections (H - Head, W - Wing, B - Body)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if len(list(self.fsc)) > 3:
raise ValueError(
"Finite size corrections require as an input a combination of H, W and B "
"for the different finite size corrections (H - Head, W - Wing, B - Body)")
for i, letter in enumerate(list(self.fsc)):
if letter not in ["H", "W", "B"]:
raise ValueError(
"Finite size corrections require as an input a combination of H, W and B "
"for the different finite size corrections (H - Head, W - Wing, B - Body)")
if any(char not in {"H", "W", "B"} for char in self.fsc):
raise ValueError(
"Finite size corrections require as an input a combination of H, W and B "
"for the different finite size corrections (H: Head, W: Wing, B: Body)")

Comment on lines +810 to +811
"""Get the finite size corrected uncompressed ``(1+ aux, MO, MO)``
integrals."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""Get the finite size corrected uncompressed ``(1+ aux, MO, MO)``
integrals."""
"""
Get the finite size corrected uncompressed ``(1+ aux, MO, MO)`` integrals.
"""

Comment on lines +459 to +464
if ct.flags.f_contiguous:
c = np.asfortranarray(ct.transpose(order_ct))
elif ct.flags.c_contiguous:
c = np.ascontiguousarray(ct.transpose(order_ct))
else:
c = ct.transpose(order_ct)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still not comfortable with this and don't think it's needed -- if there is need for contiguity it should be forced when needed and not here, is that ok?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants